Skip to content

feat: GitHubProject.getInstance(), project.getProperties()#67

Merged
gr2m merged 8 commits intomainfrom
66/add-project-data
Feb 13, 2023
Merged

feat: GitHubProject.getInstance(), project.getProperties()#67
gr2m merged 8 commits intomainfrom
66/add-project-data

Conversation

@tmelliottjr
Copy link
Copy Markdown
Collaborator

Adds databaseId and other project level properties to the GitHubProject object.

Closes #66

Copy link
Copy Markdown
Owner

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a separate method to get project data, using getters that return undefined until a certain internal state is not a good API. The new API should always behave the same to the consumer, no matter the internal state.

@tmelliottjr
Copy link
Copy Markdown
Collaborator Author

We need a separate method to get project data, using getters that return undefined until a certain internal state is not a good API. The new API should always behave the same to the consumer, no matter the internal state.

You're right! I'll rework this. I had something more akin to that originally, but ultimately went with the 'simpler' option here.

@gr2m
Copy link
Copy Markdown
Owner

gr2m commented Aug 11, 2022

Start with the the README update to show the new API, once we agree on that, the rest will be easy.

@tmelliottjr
Copy link
Copy Markdown
Collaborator Author

Start with the the README update to show the new API, once we agree on that, the rest will be easy.

Will do, my plan is to start this tomorrow, but I may not get to it until Monday.

@tmelliottjr tmelliottjr marked this pull request as draft August 16, 2022 00:02
@tmelliottjr
Copy link
Copy Markdown
Collaborator Author

@gr2m Here is the updated readme adding references for the potential new API.

Another path we can consider is making this available during initialization. This might also lend itself to some issues we're trying to solve internally. For example, instantiating with an non-existent project number provides no immediate feedback:

const project = new GitHubProject({
  token: "<token>",
  org: "<org>",
  number: 9999,
});

It's not until we attempt to list items that we realize an issue exists:

await project.items.list()
file:///github-project/api/lib/get-state-with-project-items.js:33
    projectNext.fields.nodes
                ^

TypeError: Cannot read properties of null (reading 'fields')
    at getStateWithProjectItems (file:///github-project/api/lib/get-state-with-project-items.js:33:17)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async listItems (file:///github-project/api/items.list.js:13:26)
    at async file:///github-project/.test/scratchpad.js:12:13

Node.js v18.2.0

@gr2m
Copy link
Copy Markdown
Owner

gr2m commented Aug 23, 2022

I'd suggest to merge #72 first, then create a 2.x branch, and update this PR head from main to 2.x. I'd add the change to #72.

If we merge it first then I fear it'd add a lot merge conflicts

Here is the updated readme adding references for the potential new API.

looks good 👍🏼

Another path we can consider is making this available during initialization

We could offer a factory method (e.g. a static class method such as Project.get(options)) as an alternative which would internally instantiate the project instance and run project.get().

We should definitely improve error messages either way. For a better debugging experience, we could call project.get() before using any other methods.

Removing the synchronous instantiation altogether will make it harder to test & mock.

@gr2m gr2m changed the base branch from main to 2.x August 25, 2022 18:00
@gr2m
Copy link
Copy Markdown
Owner

gr2m commented Aug 25, 2022

Should we go with Project.get() and project.get()? I fear that might be confusing, as Project.get() would resolve with a project instance, and project.get() would resolve with with an object of project properties.

Maybe Project.getInstance() and project.getProperties()?

@tmelliottjr
Copy link
Copy Markdown
Collaborator Author

Should we go with Project.get() and project.get()? I fear that might be confusing, as Project.get() would resolve with a project instance, and project.get() would resolve with with an object of project properties.

Maybe Project.getInstance() and project.getProperties()?

Definitely agree, that Project.get() and project.get() would be too confusing. Project.getInstance() and project.getProperties are perfect! I have been referring to the latter as data, but properties seems clearer and wouldn't imply that it also included items.

@gr2m
Copy link
Copy Markdown
Owner

gr2m commented Aug 25, 2022

Let me know if you plan on working on it. If not I can try to work on it myself, but can't make promises when I'll find the time right now

@tmelliottjr
Copy link
Copy Markdown
Collaborator Author

Let me know if you plan on working on it. If not I can try to work on it myself, but can't make promises when I'll find the time right now

Yes! I plan to, but I'm working through some higher priority items atm.

Comment thread README.md Outdated
@tmelliottjr tmelliottjr changed the base branch from 2.x to main February 13, 2023 16:02
@tmelliottjr tmelliottjr reopened this Feb 13, 2023
@tmelliottjr tmelliottjr marked this pull request as ready for review February 13, 2023 20:10
Comment thread README.md Outdated
Co-authored-by: Gregor Martynus <39992+gr2m@users.noreply.github.com>
@gr2m gr2m changed the title feat: adds project level properties to project object feat: GitHubProject.getInstance(), project.getProperties() Feb 13, 2023
@gr2m gr2m merged commit bb87236 into main Feb 13, 2023
@gr2m gr2m deleted the 66/add-project-data branch February 13, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add project databaseId and make project level data available

3 participants